refactor: merge job agent config in dispatcher #803

adityachoudhari26 · 2026-02-12T00:36:55Z

Summary by CodeRabbit

Refactor
- Removed capability introspection from job agents and related capability types; dispatchables now expose only type and dispatch behavior.
- Centralized job-agent configuration merging in the registry; removed local merge helpers and defaulted to empty configs in several dispatch paths.
Tests
- Added extensive unit tests for registry, merging, context population, and dispatch flows.
- Simplified factory tests to focus on error cases when no job agent is configured.

coderabbitai · 2026-02-12T00:37:11Z

📝 Walkthrough

Walkthrough

Removes the Supports() method and Capabilities type from jobagents, centralizes release/workflow context population and JobAgentConfig merging into Registry helpers, refactors Dispatch to use those helpers, and adds extensive unit tests for merging and dispatch flows.

Changes

Cohort / File(s)	Summary
Interface & Types `apps/workspace-engine/pkg/workspace/jobagents/types/types.go`	Deleted `Supports() Capabilities` from `Dispatchable` and removed the `Capabilities` type — dispatchables now only expose `Type()` and `Dispatch()`.
Job Agent Implementations `apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go`, `apps/workspace-engine/pkg/workspace/jobagents/github/githubaction.go`, `apps/workspace-engine/pkg/workspace/jobagents/terraformcloud/tfe.go`, `apps/workspace-engine/pkg/workspace/jobagents/testrunner/testrunner.go`	Removed `Supports()` methods from all job agent types; `Type()` and `Dispatch()` behavior unchanged.
Registry Layer `apps/workspace-engine/pkg/workspace/jobagents/registry.go`, `apps/workspace-engine/pkg/workspace/jobagents/registry_test.go`	Added `fillReleaseContext`, `fillWorkflowContext`, `getMergedJobAgentConfig`, `mergeJobAgentConfig`, `deepMerge`; refactored `Dispatch` to call these helpers and centralized config merging; added comprehensive tests and test doubles.
Job Factory & Helpers `apps/workspace-engine/pkg/workspace/jobs/factory.go`, `apps/workspace-engine/pkg/workspace/jobs/helpers.go`, `apps/workspace-engine/pkg/workspace/jobs/factory_test.go`	Removed `MergeJobAgentConfig` and deleted `helpers.go` (toMap, deepMerge). `CreateJobForRelease` no longer merges configs (uses empty `JobAgentConfig`); tests simplified to focus on missing/invalid agent cases.
Workflow Manager `apps/workspace-engine/pkg/workspace/workflowmanager/manager.go`	Removed local `mergeJobAgentConfig` / `deepMerge` and stopped merging `JobAgentConfig` during workflow dispatch initialization.
Test & Release Manager Updates `apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`	Added test helpers to create environments/resources and updated tests to upsert dependencies before release execution.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant Registry
participant Store
participant Dispatcher
participant JobAgent
Client->>Registry: Request Dispatch(job, ctx)
Registry->>Store: fillReleaseContext(job.ReleaseID)
Store-->>Registry: Release, Deployment, Environment, Resource, Version
Registry->>Store: fillWorkflowContext(job.WorkflowID)
Store-->>Registry: WorkflowJob, WorkflowRun
Registry->>Registry: getMergedJobAgentConfig(agent, workflow, deployment, version)
Registry->>Dispatcher: Dispatch(job with merged JobAgentConfig)
Dispatcher->>JobAgent: Type-specific Dispatch
JobAgent-->>Dispatcher: result
Dispatcher-->>Registry: result
Registry->>Store: upsert Job (with JobAgentConfig & result)
Registry-->>Client: Dispatch result

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

chore: use agent registry to dispatch jobs #764 — Overlaps registry dispatch and config-merge helpers; strong code-level overlap with the registry refactor.
chore: init workflow manager #759 — Directly related to adding/removing Supports() / Capabilities on jobagents; touches the same interface/API.
chore: make workflow jobs run concurrently #773 — Modifies workflow dispatch and job-agent config merging; overlaps with moved/centralized merge logic.

Poem

🐰
I hopped through registry paths tonight,
Collected contexts snug and light,
No more Supports to ask what you do,
Dispatch now flows — tidy and true,
Tests cheered, carrots given, moonlight bright 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main refactoring objective—moving job agent config merging logic into the dispatcher layer (Registry), which is the central change across multiple files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch merge-job-agent-config-in-dispatcher

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

apps/workspace-engine/pkg/workspace/jobagents/registry_test.go (1)
72-78: Bare type assertion after non-fatal assert will panic on failure instead of producing a clean test error.

On line 74 (and similarly line 437), if deepMerge has a bug and dst["nested"] is not map[string]any, the type assertion panics rather than reporting a clear failure. Use require for the prerequisite check or a comma-ok assertion.
Example fix for line 74
-	nested := dst["nested"].(map[string]any)
+	nested, ok := dst["nested"].(map[string]any)
+	require.True(t, ok, "expected nested to be map[string]any")
Same pattern applies at line 437.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

apps/workspace-engine/pkg/workspace/jobs/factory.go (2)
32-33: ⚠️ Potential issue | 🟡 Minor

Stale doc comment — merging no longer happens here.

The comment says "configured with merged settings from JobAgent + Deployment" but this function now sets an empty JobAgentConfig{}. The merging was moved to the registry's Dispatch flow.
Proposed fix
-// CreateJobForRelease creates a job for a given release (PURE FUNCTION, NO WRITES).
-// The job is configured with merged settings from JobAgent + Deployment.
+// CreateJobForRelease creates a job for a given release (PURE FUNCTION, NO WRITES).
+// The job is created with an empty JobAgentConfig; config merging is handled during dispatch.
119-123: ⚠️ Potential issue | 🟡 Minor

Trace step message is misleading.

"Applied default job agent configuration" implies some configuration was applied, but the job is created with an empty config. Consider updating the message to reflect that config merging is deferred to dispatch.

🤖 Fix all issues with AI agents

In `@apps/workspace-engine/pkg/workspace/jobagents/registry_test.go`:
- Around line 369-396: The test
TestGetMergedJobAgentConfig_AllSourcesMergeInOrder mocks ctx.Version directly
which masks the bug where fillReleaseContext never populates
DispatchContext.Version; add an end-to-end integration test that exercises the
real Dispatch flow (invoke Dispatch or the higher-level method that calls
fillReleaseContext) and assert that the DeploymentVersion.JobAgentConfig is
merged into the final config; ensure the new test does not set ctx.Version
manually but triggers the code paths that populate Version via
fillReleaseContext so getMergedJobAgentConfig is validated in a realistic
scenario.

In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go`:
- Line 82: Rename the misspelled variable `wofklowJobConfig` to
`workflowJobConfig` (type oapi.JobAgentConfig) and update every reference to it
(including the usage around the later code noted at line ~97) so the identifier
is consistent; ensure you only change the identifier name and keep the type and
initialization intact.
- Around line 40-57: In fillReleaseContext, after assigning ctx.Release,
ctx.Deployment, ctx.Environment, and ctx.Resource, also set ctx.Version from the
release (e.g. ctx.Version = jobWithRelease.Release.Version) so version-level
config merging in getMergedJobAgentConfig will run; additionally correct the
typo `wofklowJobConfig` to `workflowJobConfig` wherever that identifier appears.

🧹 Nitpick comments (2)

apps/workspace-engine/pkg/workspace/jobagents/registry.go (1)

139-145: mergeJobAgentConfig never returns an error — signature is misleading.

The function always returns nil for the error. Either simplify the signature to return only oapi.JobAgentConfig, or document why the error return is kept (e.g., future-proofing for JSON-based merging).

apps/workspace-engine/pkg/workspace/jobs/factory_test.go (1)

28-66: Remove unused typed config helper functions.

getGithubJobAgentConfig, getArgoCDJobAgentConfig, getTerraformCloudJobAgentConfig, and getTestRunnerJobAgentConfig are not called anywhere in the codebase and appear to be remnants of removed tests. Remove them.

apps/workspace-engine/pkg/workspace/jobagents/registry_test.go

apps/workspace-engine/pkg/workspace/jobagents/registry.go

jsbroks · 2026-02-12T01:41:16Z

@cursoragent could you review the pull and explain why capability introspection from job agents is no longer needed

cursor · 2026-02-12T01:41:24Z

Here’s the short version: capability introspection was removed in PR #803 (“refactor: merge job agent config in dispatcher”) to simplify the job-agent interface and centralize config/dispatch logic.

• Simplified interface: Dropped Capabilities and the Supports() method from job agents; Dispatchable now only requires Type() and Dispatch(...).
• Single source of truth: Merged job-agent config is now computed centrally in the registry (agent + workflow job + deployment + version) right before dispatch, avoiding duplicated merge logic and drift.
• Removed fragile gating: The registry no longer gates by “workflows vs deployments” based on introspection; validation is handled earlier (e.g., factory marks missing job agents invalid), and dispatchers are expected to work with the unified context.
• Tests reflect this: Heavy additions to registry_test.go; factory/workflow tests updated to focus on erroring when no job agent is configured.

Code references:

type Dispatchable interface {
  Type() string
  Dispatch(ctx context.Context, context DispatchContext) error
}

func (r *Registry) Dispatch(ctx context.Context, job *oapi.Job) error {
  jobAgent, ok := r.store.JobAgents.Get(job.JobAgentId)
  ...
  mergedConfig, err := r.getMergedJobAgentConfig(jobAgent, &dispatchContext)
  ...
  job.JobAgentConfig = mergedConfig
  r.store.Jobs.Upsert(ctx, job)
  return dispatcher.Dispatch(ctx, dispatchContext)
}

job := &oapi.Job{
  ...
  JobAgentConfig: oapi.JobAgentConfig{},  // now defaulted; merged later in registry
  ...
}

Status: I reviewed PR #803 and its diff to confirm capability introspection removal and the centralization of config merging in the registry; no blockers found.

refactor: merge job agent config in dispatcher

cb03c9c

coderabbitai bot reviewed Feb 12, 2026

View reviewed changes

apps/workspace-engine/pkg/workspace/jobagents/registry_test.go Show resolved Hide resolved

apps/workspace-engine/pkg/workspace/jobagents/registry.go Show resolved Hide resolved

apps/workspace-engine/pkg/workspace/jobagents/registry.go Outdated Show resolved Hide resolved

adityachoudhari26 added 2 commits February 11, 2026 16:49

rabbit comments

6fdfa50

lint

bffd915

reorder

4798fab

adityachoudhari26 merged commit be95718 into main Feb 12, 2026
9 checks passed

adityachoudhari26 deleted the merge-job-agent-config-in-dispatcher branch February 12, 2026 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: merge job agent config in dispatcher #803

refactor: merge job agent config in dispatcher #803

Uh oh!

adityachoudhari26 commented Feb 12, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Feb 12, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsbroks commented Feb 12, 2026 •

edited

Loading

Uh oh!

cursor bot commented Feb 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refactor: merge job agent config in dispatcher #803

refactor: merge job agent config in dispatcher #803

Uh oh!

Conversation

adityachoudhari26 commented Feb 12, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsbroks commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor bot commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adityachoudhari26 commented Feb 12, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 12, 2026 •

edited

Loading

jsbroks commented Feb 12, 2026 •

edited

Loading

cursor bot commented Feb 12, 2026 •

edited

Loading